Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

join: join over lb if available #2348

Merged
merged 2 commits into from
Sep 25, 2023
Merged

join: join over lb if available #2348

merged 2 commits into from
Sep 25, 2023

Conversation

3u13r
Copy link
Member

@3u13r 3u13r commented Sep 19, 2023

Context

This is needed since the upcoming node-to-node strict mode does not allow traffic which is not WireGuard encrypted by Cilium (with the exception of etcd). Therefore nodes cannot join via directly talking to another node.

Proposed change(s)

  • add join port to all LBs
  • Let rejoinclient and joinclient join over LB (if available, otherwise fallback to node VPC IPs)

The fallback is needed to still support Miniconstellation as it does not have an LB.

e2e upgrade tests:
azure: https://github.com/edgelesssys/constellation/actions/runs/6237449241
gcp: https://github.com/edgelesssys/constellation/actions/runs/6238824048
aws: https://github.com/edgelesssys/constellation/actions/runs/6238826460

Checklist

  • Add labels (e.g., for changelog category)
  • Link to Milestone

@netlify
Copy link

netlify bot commented Sep 19, 2023

Deploy Preview for constellation-docs canceled.

Name Link
🔨 Latest commit 7868347
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/650d5cff979d1e000855c618

@github-actions
Copy link
Contributor

github-actions bot commented Sep 19, 2023

Coverage report

Package Old New Trend
bootstrapper/cmd/bootstrapper [no test files] [no test files] 🚧
bootstrapper/internal/joinclient 89.30% 88.50% ↘️
disk-mapper/internal/rejoinclient 91.80% 93.10% ↗️
disk-mapper/internal/setup 68.90% 68.90% ↔️
internal/cloud/metadata [no test files] [no test files] 🚧

@3u13r 3u13r added this to the v2.12.0 milestone Sep 19, 2023
@3u13r 3u13r added the no changelog Change won't be listed in release changelog label Sep 19, 2023
@3u13r 3u13r marked this pull request as ready for review September 19, 2023 16:48
@elchead
Copy link
Contributor

elchead commented Sep 20, 2023

could you add the context why we need this?

@elchead
Copy link
Contributor

elchead commented Sep 20, 2023

can u also provide a run for miniconstellation?

@elchead
Copy link
Contributor

elchead commented Sep 20, 2023

looked mostly over the changes regarding TF.

bootstrapper/internal/joinclient/joinclient.go Outdated Show resolved Hide resolved
disk-mapper/internal/rejoinclient/rejoinclient.go Outdated Show resolved Hide resolved
Comment on lines -47 to -48
// JoinServiceEndpoints returns the list of endpoints for the join service, which are running on the control plane nodes.
func JoinServiceEndpoints(ctx context.Context, lister InstanceLister) ([]string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since joinclient.go and rejoinclient.go have to perform the same action to fetch loadbalancer endpoint, and control plane IPs, what about adapting this function to include fetching of LB endpoint and using it for both?
With the current implementation, the code is slightly different for joinclient.go and rejoinclient.go.

Function could look like the following:

type endpointLister interface {
    List(ctx context.Context) ([]InstanceMetadata, error)
    GetLoadBalancerEndpoint(ctx context.Context) (host, port string, err error)
}

func JoinServiceEndpoints(ctx context.Context, lister endpointLister) ([]string, error) {
    lbIP, _, err := lister.GetLoadBalancerEndpoint(ctx)
    if err != nil {
        // Maybe do additional error handling here in case of no loadbalancer (ignore error?)
        return nil, fmt.Errorf("retrieving load balancer endpoint from cloud provider: %w", err)
    }
    // Old JoinServiceEndpoints code
    joinEndpoints := []string{}
    ...

    // Return all endpoints, with LB as the first endpoint
    return append(net.JoinHostPort(lbEndpoint, strconv.Itoa(constants.JoinServiceNodePort)), joinEndpoints), nil
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I didn't like the place of this function as part of the metadata package as it is too specific to me. Also, without this function the metadata package looks cleaner.
I see you point of introducing a potential difference in both implementation though, but I haven't found the right spot.

Copy link
Contributor

@elchead elchead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

started a mini test and lgtm when green

@elchead elchead temporarily deployed to e2e September 22, 2023 13:39 — with GitHub Actions Inactive
@3u13r 3u13r merged commit 2776e40 into main Sep 25, 2023
9 checks passed
@3u13r 3u13r deleted the feat/arch/join-over-lb branch September 25, 2023 08:23
msanft pushed a commit that referenced this pull request Sep 27, 2023
* join: join over lb if available
derpsteb pushed a commit that referenced this pull request Oct 2, 2023
* join: join over lb if available
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Change won't be listed in release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants